Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow vector patterns to be used in kernels #38

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

croyzor
Copy link
Collaborator

@croyzor croyzor commented Oct 14, 2024

  • Add our vector patterns to the map of allowed kernel constructors (for kernel Vec)
  • Add an example file examples/brick.brat
  • Update detectVecErrors to deal with the existence of more complicated vector tests than "0 or succ?". This is very much a bandage over a system that needs to be completely redone, but I hit up against this trying to write the example and it's errors should be slightly more informative now

@croyzor croyzor requested a review from acl-cqc October 14, 2024 15:14
toLenConstr NP0 = pure $ Length 0
toLenConstr (NP1Plus (NP2Times np)) = either (const (Right LengthOdd)) Right (toLenConstr np)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok...so....what does this do.....

  • If toLenConstr np returns (Left error), we discard the error and return pure LengthOdd
  • If toLenConstr np returns a Right value, we leave it unchanged

Is that right? (This is quite hard to follow)

If so, how about pure $ fromRight LengthOdd (toLenConstr np) ?

err $ getVecErr tp (show ty) (show n) p'
Left (NumMatchFail _ _) -> case (toLenConstr p) of
Right p' -> err $ getVecErr tp (show ty) (show n) p'
Left p' -> err $ InternalError ("detectVecErrors: Unexpected pattern: " ++ show (toLenConstr p'))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this toLenConstr p'? This seems odd. So after toLenConstr has stripped away any outer (NP1Plus?) NPTwoTimess from the input p to give p', we then call it again to construct an error message? Is that right?

I'm hoping this should/could just be show p' in which case I have a different suggestion - how about moving the error-construction into toLenConstr (so it's :: NumPat -> Either Error LengthConstraint) and then using throwLeft here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it's a bit hard to follow as is

toLenConstr :: NumPat -> Checking LengthConstraint
-- Try to work out the length of a vector
-- We only want to know if the vector length is nil or a successor
toLenConstr :: NumPat -> Either NumPat (LengthConstraint)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superfluous extra brackets here, at least

@@ -0,0 +1,20 @@
-- Create a "brickwork" state
-- Apply a parameterised unitary U to entangle every adjacent pair of qubits in a line architecture.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eeek - skipping dependent types to ensure weights match in length, is this roughly just
brick(th ,- ths, U) = {q0 ,- q1 ,- qrest => let (q0,q1) = U(th)(q0,q1) in q0 ,- brick(ths, U)(q1 ,- rest)? (I.e. gate the first two qubits, then proceed "down the line" - what I thought was called a "ladder" architecture)?

If so then whilst a fun bit of list comprehensions they don't really do much useful (you're just riffling things apart and back together, and so on), right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're right, it's just a bit of fun that tests vector patterns are working in kernels 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terminology-wise, I think this is called a brickwork pattern and a ladder is when everything else is sequentially entangled with 1 qubit

@@ -121,6 +121,28 @@ kernelConstructors = M.fromList
(RPr ("tail", TVec (VApp (VInx (VS VZ)) B0) (VNum $ nVar (VInx VZ)))
(RPr ("head", VApp (VInx (VS VZ)) B0) R0)))
])
,(CConcatEqEven, M.fromList
[(CVec, CArgs [VPVar, VPNum (NP2Times NPVar)] (Sy (Sy Zy))
-- Star should be a TypeFor m forall m?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure I understand this comment...what happens if you replace the star with that TypeFor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try and push something to clear this up

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was that you could split on classical variables in the kernel, like

divConquer(n :: #, Vec({ Qubit -o Qubit }, n)) -> { Vec(Qubit, n) -o Vec(Qubit, n) }
divConquer(0, []) = { .. }
divConquer(doub(n), gs) = { qs0 =,= qs1 =>
  let gs0 =,= gs1 = gs in divConquer(n, xs0)(gs0) =,= divConquer(n, gs1)(qs1)
}

but having just tried that, there are more blockers (gs isn't in scope in a kernel context anyway!) so I'll remove the comments.
Also, it'd be nice to be able to do this, as long as the pattern match is irrefutable... I guess we should come back to it after we have checking coverage of patterns

Left (NumMatchFail _ _) -> do
p' <- toLenConstr p
err $ getVecErr tp (show ty) (show n) p'
Left (NumMatchFail _ _) -> case (toLenConstr p) of
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So whilst continuing the slightly-hacked-in nature here and a better framework might be nice I don't think this is all that bad, certainly I'm not objecting to patching it in like this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants